[Logs] OpenTelemetry.Extensions.EventSource extensions project #3454

CodeBlanch · 2022-07-18T17:34:46Z

[Goal is to pare down the amount of changes on #3305]

Changes

Adds a project which exposes an EventListener for emitting OpenTelemetry logs written via EventSource

Notes

This project is currently part of the core repo because all of the LogEmitter artifacts are internal. Once those go public this should be moved to contrib repo.
The goal here is to release this project as an experimental/preview thing to gather feedback.

TODOs

Appropriate CHANGELOG.md updated for non-trivial changes
Unit tests

CodeBlanch · 2022-07-18T17:38:01Z

src/OpenTelemetry.Extensions.Tracing/OpenTelemetry.Extensions.Tracing.csproj

+  <PropertyGroup>
+    <!-- OmniSharp/VS Code requires TargetFrameworks to be in descending order for IntelliSense and analysis. -->
+    <TargetFrameworks>netstandard2.1;netstandard2.0</TargetFrameworks>
+    <Description>Extensions for using OpenTelemetry with System.Diagnostics.Tracing</Description>


I know the name OpenTelemetry.Extensions.Tracing is going to be a lightning rod but I'll explain my thinking here.

My intention was this project would contain extensions for things in the System.Diagnostics.Tracing namespace. EventSource but also EventCounters. IMO OpenTelemetry.Instrumentation.EventCounters is misnamed.

Anyway open to alternatives just thought I would explain myself 😄

My intention was this project would contain extensions for things in the System.Diagnostics.Tracing namespace.

Then we could probably add Diagnostics in the package name: OpenTelemetry.Extensions.Diagnostics.Tracing because just OpenTelemetry.Extensions.Tracing might suggest that these would be extensions for OTel Traces covering things such as AutoFlushActivityProcessor

⚡ ⚡ !! Haha, yea naming is a bummer. We should all just stop using words.

Understood that System.Diagnostics.Tracing namespace involves more than EventSource, but were you envisioning that this package might one day involve itself with EventCounters somehow?

OpenTelemetry.Extensions.EventSource was my favorite of my suggestions here
#3305 (comment). But I could get behind the OpenTelemetry.Extensions.Diagnostics.Tracing or even more precisely including the whole namespace OpenTelemetry.Extensions.System.Diagnostics.Tracing ... even though it gives me a minor headache 🥴.

Could we have separate packages for OpenTelemetry.Extensions.EventCounter and OpenTelemetry.Extensions.EventSource? (e.g. I guess there are far more developers who need EventSource integration comparing to EventCounter)

I prefer separate packages because they can be deprecated independently.

OK I'll rename this OpenTelemetry.Extensions.EventSource and get to work on tests. @utpilla You want me to open issue to rename OpenTelemetry.Instrumentation.EventCounters -> OpenTelemetry.Extensions.EventCounter?

[This reply is to @utpilla + @cijothomas I had it open for a while and didn't see @alanwest sneak in there. I'm good with punting on this discussion.]

Hmm interesting. Hard time wrapping my mind around this. Is this definition from the spec somewhere? User plugging this stuff in is doing so at the host level so I'm not sure if the API/SDK distinction is useful.

Just eyeballing this list...

Which thing is not like the others? 🍎 🍎 🍎 🍌 🤣

OpenTelemetry.Instrumentation.AspNet - Emits telemetry about AspNet
OpenTelemetry.Instrumentation.MassTransit - Emits telemetry about MassTransit
OpenTelemetry.Instrumentation.Runtime - Emits telemetry about.NET runtime
OpenTelemetry.Instrumentation.StackExchangeRedis - Emits telemetry about Redis
...etc...
OpenTelemetry.Instrumentation.EventCounters - Transposes telemetry written about whatever using EventCounters into System.Diagnostics.DiagnosticSource telemetry. Does not emit telemetry about EventCounters

IMO the HOW it is done is less important than WHAT it is doing.

Is this definition from the spec somewhere?

No. My view !
Only spec defined term is https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/glossary.md#instrumentation-library

We need to definitely discuss this more.

IMO the HOW it is done is less important than WHAT it is doing.

💯

Yes 💯 to that as well. I think I've captured the spirit of WHAT in the issue I just opened #3469.

Also, per my thoughts in #3469, I've changed my mind and I think this package should be OpenTelemetry.Shims.EventSource 😆.

Which thing is not like the others? 🍎 🍎 🍎 🍌 🤣

Also, rename OpenTelemetry.Extensions.EventCounter -> OpenTelemetry.🍌

codecov · 2022-07-18T17:48:20Z

Codecov Report

Merging #3454 (f02ec69) into main (795aedc) will increase coverage by 0.29%.
The diff coverage is 96.20%.

@@            Coverage Diff             @@
##             main    #3454      +/-   ##
==========================================
+ Coverage   86.10%   86.40%   +0.29%     
==========================================
  Files         269      270       +1     
  Lines        9712     9791      +79     
==========================================
+ Hits         8363     8460      +97     
+ Misses       1349     1331      -18

Impacted Files	Coverage Δ
....EventSource/OpenTelemetryEventSourceLogEmitter.cs	`96.20% <96.20%> (ø)`
...metryProtocol/Implementation/ActivityExtensions.cs	`94.50% <0.00%> (+3.29%)`	⬆️
...xporter.OpenTelemetryProtocol/OtlpTraceExporter.cs	`77.27% <0.00%> (+40.90%)`	⬆️
...entation/ExportClient/OtlpGrpcTraceExportClient.cs	`78.57% <0.00%> (+42.85%)`	⬆️

src/OpenTelemetry.Extensions.Tracing/OpenTelemetryEventSourceLogEmitter.cs

src/OpenTelemetry.Extensions.Tracing/README.md

src/OpenTelemetry.Extensions.EventSource/README.md

cijothomas · 2022-07-21T19:18:27Z

examples/LoggingExtensions/Program.cs

+// names matching OpenTelemetry* into logs
+using var openTelemetryEventSourceLogEmitter = new OpenTelemetryEventSourceLogEmitter(
+    openTelemetryLoggerProvider, // <- Events will be written to openTelemetryLoggerProvider
+    (name) => name.StartsWith("OpenTelemetry") ? EventLevel.LogAlways : null,


Was it intentional that "OpenTelemetry" was used? any risk of cyclic/loops?
Might be good to create a custom EventSource-Foo or something, and use that in examples?

Ya it was intentional. I just added this comment to explain the behavior:

opentelemetry-dotnet/examples/LoggingExtensions/Program.cs

Lines 56 to 58 in 8397470

// Manually dispose OpenTelemetryLoggerProvider since it is being shared. This

// causes a log message to be written to the OpenTelemetry-Sdk EventSource which

// OpenTelemetryEventSourceLogEmitter will capture.

I'm open to changing it LMK what you think about that.

Doesn't have any cyclic or loop issues currently as OpenTelemetry.Extensions.EventSource doesn't have its own EventSource.

I see.
I meant this when I said cyclic loop:

If I enable this EventSourceLogEmitter, it'll subscribe to all ES starting with OpenTelemetry, and pipes them to OTel Logging SDK.

OTel Logging SDK internally uses ES whose name starts with OpenTelemetry. So that EventSource Logs gets piped through this LoggingSDK again, which inturn produce more log....

(or did i got it wrong fully 🏃 )

The enduser for this package is people who already EventSource for their logging, and want those piped through OTel logging. For such use-cases, the best would be to show a custom EventSource, logging to it, but it showing up in OTel logging.
Similar to the example for Serilog...

Updated the example to use a custom source.

For circular logging, I think we may have a bigger issue. Opened [Discussion] [Logs] Circular logging #3470 to discuss.

src/OpenTelemetry.Extensions.EventSource/OpenTelemetry.Extensions.EventSource.csproj

src/OpenTelemetry.Extensions.EventSource/OpenTelemetryEventSourceLogEmitter.cs

cijothomas

LGTM.
Left a comment about the example - I think it should show a custom EventSource (and not OpenTelemetry event source!)

CodeBlanch · 2022-07-21T21:11:01Z

examples/LoggingExtensions/Program.cs


-// Note: It is important that OpenTelemetryLoggerProvider is disposed when the
-// app is shutdown. In this example we allow Serilog to do that by calling CloseAndFlush.
 var openTelemetryLoggerProvider = new OpenTelemetryLoggerProvider(options =>


@alanwest @cijothomas @utpilla One thing I realized working on tests. The Serilog + EventSource APIs ask you to pass in the OpenTelemetryLoggerProvider. If you are creating it manually, that is super easy to do. However if you are using the ILoggingBuilder extension it is tricky/non-obvious how you get the actual OpenTelemetryLoggerProvider. Maybe as a follow-up I'll see if we can smooth that out somehow?

alanwest

LGTM if you can get it to build 😄.

I really do think we should consider using the term Shims over Extensions but fine to follow up with that rename if folks want to mull it over more.

CodeBlanch · 2022-07-21T23:54:44Z

src/OpenTelemetry.Extensions.EventSource/OpenTelemetryEventSourceLogEmitter.cs

+
+            LogRecordAttributeList attributes = default;
+
+            attributes.Add("event_source.name", eventData.EventSource.Name);


@cijothomas @alanwest @reyang @utpilla Right now EventSource.Name becomes the event_source.name tag. CategoryName is null. Anyone feel like EventSource.Name should end up the CategoryName and we should drop the tag? I kind of feel like that would be a good change.

hmm. Well the goofy thing about CategoryName is that it is an ILogger concept. We've taken the stance to kind of treat ILogger as special in OpenTelemetry libraries. I'm not sure it makes sense to try to coerce other logging libraries concepts into ILogger concepts.

From the standpoint of OTLP we currently transform CategoryName into an attribute named dotnet.ilogger.category.

opentelemetry-dotnet/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/LogRecordExtensions.cs

Lines 78 to 86 in cf3dfc5

if (!string.IsNullOrEmpty(logRecord.CategoryName))

{

// TODO:

// 1. Track the following issue, and map CategoryName to Name

// if it makes it to log data model.

// https://github.com/open-telemetry/opentelemetry-specification/issues/2398

// 2. Confirm if this name for attribute is good.

otlpLogRecord.Attributes.AddStringAttribute("dotnet.ilogger.category", logRecord.CategoryName);

}

It might actually make more sense to abandon the use of CategoryName and add an appropriately named attribute to the LogRecord upstream just like you've done here with event_source.name. If some some other extension author ends up using CategoryName somehow, then It think in the context of OTLP we should just map that to an attribute named dotnet.otel.category_name or something.

Also regarding the comment

// 1.Track the following issue, and map CategoryName to Name
// if it makes it to log data model.

The Name field did not make it into the data model: open-telemetry/opentelemetry-proto#357, and I think at this point in time we should assume a world where it never comes back. And even if it did, I'm still beginning to think that attributes named in a way that reflect the logging library that was used is more correct.

You convinced me to just leave it alone (as event_source.name tag).

FYI in the Serilog sink I do have this code...

opentelemetry-dotnet/src/OpenTelemetry.Extensions.Serilog/OpenTelemetrySerilogSink.cs

Lines 71 to 75 in bfabe9b

if (property.Key == Constants.SourceContextPropertyName

&& property.Value is ScalarValue sourceContextValue)

{

data.CategoryName = sourceContextValue.Value as string;

}

...which will set CategoryName when users use the ForContext API...

ILogger programLogger = Log.Logger.ForContext<Program>(); // <- CategoryName will be essentially typeof(Program).FullName

Seemed like a good fit to me but we may want to change that?

Sorry, didn't see your comment here... yes, we need to consider what to do. Opened #3491 for discussion.

I'm not familiar enough with Serilog to make a suggestion here. Seems like a serilog.context attribute may make sense? Though, a quick search over Serilog code leads me to believe ForContext may be a bit more complex than just a name - look at this for example: https://github.com/serilog/serilog/blob/697a23189e3068bfe0da1460a2676913239d2757/src/Serilog/LoggerExtensions.cs#L24-L29... though maybe this is more akin to ILogger scopes?

CodeBlanch · 2022-07-22T22:37:37Z

@alanwest

LGTM if you can get it to build 😄.

Fine. It now builds too! What you want from me? 🤣

Add OpenTelemetryEventSourceLogEmitter.

eeae428

CodeBlanch requested a review from a team July 18, 2022 17:34

CodeBlanch commented Jul 18, 2022

View reviewed changes

utpilla reviewed Jul 18, 2022

View reviewed changes

src/OpenTelemetry.Extensions.Tracing/OpenTelemetryEventSourceLogEmitter.cs Outdated Show resolved Hide resolved

vishweshbankwar reviewed Jul 19, 2022

View reviewed changes

src/OpenTelemetry.Extensions.Tracing/README.md Outdated Show resolved Hide resolved

Project rename.

44a3ad8

CodeBlanch changed the title ~~[Logs] System.Diagnostics.Tracing extensions project~~ [Logs] OpenTelemetry.Extensions.EventSource extensions project Jul 21, 2022

alanwest reviewed Jul 21, 2022

View reviewed changes

src/OpenTelemetry.Extensions.EventSource/README.md Outdated Show resolved Hide resolved

cijothomas reviewed Jul 21, 2022

View reviewed changes

alanwest reviewed Jul 21, 2022

View reviewed changes

src/OpenTelemetry.Extensions.EventSource/OpenTelemetry.Extensions.EventSource.csproj Outdated Show resolved Hide resolved

CodeBlanch added 2 commits July 21, 2022 12:56

Started tests.

c6c606f

Added behavior comment in example.

8397470

cijothomas reviewed Jul 21, 2022

View reviewed changes

src/OpenTelemetry.Extensions.EventSource/OpenTelemetryEventSourceLogEmitter.cs Show resolved Hide resolved

cijothomas approved these changes Jul 21, 2022

View reviewed changes

CodeBlanch commented Jul 21, 2022

View reviewed changes

CodeBlanch added 2 commits July 21, 2022 14:12

Add --prerelease in READMEs.

3c78cd4

Example tweaks.

dcb1c83

alanwest approved these changes Jul 21, 2022

View reviewed changes

CodeBlanch mentioned this pull request Jul 21, 2022

[Discussion] [Logs] Circular logging #3470

Closed

CodeBlanch commented Jul 21, 2022

View reviewed changes

CodeBlanch added 4 commits July 21, 2022 17:16

Test improvements.

7446ec0

Added activity_id/related_activity_id test.

15f55ef

Merge branch 'main' into logemitter-tracing-extensions

dae7579

Warning cleanup.

f02ec69

CodeBlanch merged commit bfabe9b into open-telemetry:main Jul 22, 2022

CodeBlanch deleted the logemitter-tracing-extensions branch July 22, 2022 22:40

This was referenced Jul 26, 2022

[Logs] Add extension for registering OpenTelemetryLoggerProvider with ILoggingBuilder #3489

Merged

[Logs] Support dependency injection in logging build-up #3504

Merged

This was referenced Sep 29, 2022

[Logs] Revert logging API additions #3702

Merged

[Logs-branch] Add LoggerProvider spec changes #3707

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Logs] OpenTelemetry.Extensions.EventSource extensions project #3454

[Logs] OpenTelemetry.Extensions.EventSource extensions project #3454

CodeBlanch commented Jul 18, 2022 •

edited

Loading

CodeBlanch Jul 18, 2022

utpilla Jul 18, 2022

alanwest Jul 21, 2022

reyang Jul 21, 2022

CodeBlanch Jul 21, 2022

CodeBlanch Jul 21, 2022 •

edited

Loading

cijothomas Jul 21, 2022

reyang Jul 21, 2022

alanwest Jul 21, 2022

alanwest Jul 21, 2022

codecov bot commented Jul 18, 2022 •

edited

Loading

cijothomas Jul 21, 2022

CodeBlanch Jul 21, 2022

cijothomas Jul 21, 2022

cijothomas Jul 21, 2022

CodeBlanch Jul 21, 2022

cijothomas left a comment

CodeBlanch Jul 21, 2022

alanwest left a comment

CodeBlanch Jul 21, 2022

alanwest Jul 22, 2022 •

edited

Loading

alanwest Jul 22, 2022 •

edited

Loading

CodeBlanch Jul 22, 2022

alanwest Jul 26, 2022

CodeBlanch commented Jul 22, 2022

	// Manually dispose OpenTelemetryLoggerProvider since it is being shared. This
	// causes a log message to be written to the OpenTelemetry-Sdk EventSource which
	// OpenTelemetryEventSourceLogEmitter will capture.


		LogRecordAttributeList attributes = default;

		attributes.Add("event_source.name", eventData.EventSource.Name);

	if (!string.IsNullOrEmpty(logRecord.CategoryName))
	{
	// TODO:
	// 1. Track the following issue, and map CategoryName to Name
	// if it makes it to log data model.
	// https://github.com/open-telemetry/opentelemetry-specification/issues/2398
	// 2. Confirm if this name for attribute is good.
	otlpLogRecord.Attributes.AddStringAttribute("dotnet.ilogger.category", logRecord.CategoryName);
	}

	if (property.Key == Constants.SourceContextPropertyName
	&& property.Value is ScalarValue sourceContextValue)
	{
	data.CategoryName = sourceContextValue.Value as string;
	}

[Logs] OpenTelemetry.Extensions.EventSource extensions project #3454

[Logs] OpenTelemetry.Extensions.EventSource extensions project #3454

Conversation

CodeBlanch commented Jul 18, 2022 • edited Loading

Changes

Notes

TODOs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CodeBlanch Jul 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 18, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cijothomas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanwest left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanwest Jul 22, 2022 • edited Loading

Choose a reason for hiding this comment

alanwest Jul 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CodeBlanch commented Jul 22, 2022

CodeBlanch commented Jul 18, 2022 •

edited

Loading

CodeBlanch Jul 21, 2022 •

edited

Loading

codecov bot commented Jul 18, 2022 •

edited

Loading

alanwest Jul 22, 2022 •

edited

Loading

alanwest Jul 22, 2022 •

edited

Loading